Sparse reshape op #7125
Sparse reshape op #7125
Conversation
|
|
||
| def sparsereshape(sparse_indices, sparse_values, prev_shape, new_shape): | ||
| """ | ||
| Reshape a Sparse Tensor |
There was a problem hiding this comment.
Could you note that this function only support tensors in COO format, not CSR. In other parts of the codebase, we tend to use CSR.
There was a problem hiding this comment.
Can you explain how this convention is different from the sparse_to_dense operator. I could only find that operator as an example of existing representations ?
There was a problem hiding this comment.
The convention is the same as sparse_to_dense. However sparse_dense uses CSR and BSR formats. We should probably add documentation to sparse_to_dense too.
|
@tkonolige @mbrookhart I have added the TF Frontend code. Can I get a re-review on this PR following up on our discussion last week ? |
|
Style and organization-wise LGTM. @tkonolige @mbrookhart Can you PTAL? |
|
I'm out on leave this week, so I won't be giving it a full review until Monday. @codeislife99 , do you have a branch where you tested the TF model with all three ops? |
|
Thats understandable ofcourse, I can wait for the full review. In the meantime I will create a branch with all the three Ops and E2E testing. |
There was a problem hiding this comment.
I don't think this will work with dynamic input shapes, and from our conversation, my understanding is that you expect a chain of these ops with dynamic slicing in between them in the model you're targeting. I don't want to merge this until we've validated that actually works.
Otherwise, things look good.
| int new_shape_size = GetConstInt(new_shape->shape[0]); | ||
| int prev_shape_size = GetConstInt(prev_shape->shape[0]); |
There was a problem hiding this comment.
My main complaint is that this will fail with dynamic input shapes. From what I understand, you expect multiple chained dynamically-shaped sparse ops in the model you're trying to target, so I'm hesitant to merge this because I'm under the impression that this will not solve the larger problem you're trying to solve.
I'd really like to see you either test the model in a branch containing all three of your PRs, or write a unit test with a representative subgraph.
This PR is for adding support for sparse reshape OP (https://www.tensorflow.org/versions/r1.15/api_docs/python/tf/sparse/reshape) as a part of a larger effort to add sparse operator support. (#7126, #7149)